Refactoring opencode integration for shared workflows - #50145
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
Imported engines currently leak through the singleton registry, making lock output order-dependent, and supported import forms can still fail early validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Refactors OpenCode from a built-in engine into an imported, behavior-defined shared workflow.
Changes:
- Removes OpenCode from built-in registries, constants, CLI help, tests, and documentation.
- Adds shared OpenCode network/runtime configuration and updates its smoke workflow.
- Defers engine validation until imports are processed and invalidates manifest caches after registration.
File summaries
| File | Description |
|---|---|
specs/aw-harness.md |
Removes built-in OpenCode harness entry. |
pkg/workflow/spec_test.go |
Updates documented engine expectations. |
pkg/workflow/pi_mcp.go |
Generalizes MCP path commentary. |
pkg/workflow/model_env_vars_test.go |
Removes built-in OpenCode model test. |
pkg/workflow/mcp_renderer_factory.go |
Updates renderer documentation. |
pkg/workflow/engine_model_resolution_test.go |
Removes OpenCode defaults tests. |
pkg/workflow/engine_definition_test.go |
Updates built-in catalog expectations. |
pkg/workflow/engine_definition_loader.go |
Removes built-in definition lookup cache. |
pkg/workflow/engine_definition_loader_test.go |
Removes OpenCode loader expectation. |
pkg/workflow/engine_config_dir.go |
Updates built-in directory documentation. |
pkg/workflow/engine_catalog_test.go |
Updates catalog engine lists. |
pkg/workflow/engine_auth_test.go |
Updates built-in authentication tests. |
pkg/workflow/compiler_yaml_step_lifecycle.go |
Removes OpenCode-specific model variable handling. |
pkg/workflow/compiler_yaml_audit_step.go |
Updates audit documentation. |
pkg/workflow/compiler_orchestrator_engine.go |
Defers engine validation when imports exist. |
pkg/workflow/agentic_engine.go |
Removes built-in OpenCode and invalidates manifest caches. |
pkg/workflow/agent_validation.go |
Generalizes universal-consumer validation. |
pkg/workflow/agent_validation_model_test.go |
Constructs OpenCode as a behavior engine. |
pkg/parser/schema_errors.go |
Removes OpenCode from built-in schema hints. |
pkg/constants/version_constants.go |
Removes the built-in OpenCode version. |
pkg/constants/constants.go |
Generalizes OpenCode-specific comments. |
pkg/constants/constants_test.go |
Updates built-in engine expectations. |
pkg/cli/workflow_secrets_test.go |
Removes built-in OpenCode secret test. |
pkg/cli/secrets_command_test.go |
Updates bootstrap engine help test. |
pkg/cli/logs_filtering_test.go |
Updates logs help expectation. |
pkg/cli/flags.go |
Removes OpenCode from engine help. |
pkg/cli/completions_test.go |
Updates engine completions. |
pkg/cli/completion_command.go |
Updates completion documentation. |
pkg/cli/add_wizard_command.go |
Removes OpenCode from wizard help. |
pkg/cli/add_wizard_command_test.go |
Updates wizard help test. |
docs/src/content/docs/setup/cli.md |
Updates documented bootstrap engines. |
docs/public/editor/autocomplete-data.json |
Updates built-in engine autocomplete. |
.github/workflows/smoke-opencode.md |
Imports the shared OpenCode definition. |
.github/workflows/shared/opencode.md |
Adds declarative networking and usage guidance. |
.github/workflows/*.lock.yml |
Regenerates affected workflows without globally built-in OpenCode manifests. |
Review details
- Files reviewed: 284/284 changed files
- Comments generated: 3
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // | ||
| // Pi uses ShellMcpServersJsonPath (same as Claude/Gemini/Antigravity) because | ||
| // the Pi CLI resolves its MCP config from the shell environment path. OpenCode | ||
| // the Pi CLI resolves its MCP config from the shell environment path. Behavior-defined engines |
| // Invalidate the pre-computed manifest caches so engines registered after | ||
| // construction (e.g. behavior-defined engines imported from shared workflows) | ||
| // contribute their manifest files and folders. | ||
| r.cachedManifestFolders = nil | ||
| r.cachedManifestFiles = nil |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — No Test Changes
Test Quality Sentinel skipped — no behavioral tests to analyze. Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — commenting only; the diff is truncated at 3 000 lines (284 changed files) so the Go source changes in pkg/workflow/ are not visible in the patch.
📋 Key Themes & Highlights
Observations
- The lock-file and docs changes are mechanically consistent:
opencodeis removed everywhere — engine docs, sparse-checkout lists, and theGH_AW_AGENT_FILES/GH_AW_AGENT_FOLDERSenv vars in all 280+ workflows. - The docs update in
.github/aw/syntax-engine.mdcleanly removes theopencodeentry from both the string-format list and the object-format comment. ✅ - Three existing inline comments (ids 3708882025, 3708882042, 3708882060) already flag the high-impact Go-level issues: import-type bypass narrowness, grammar in
pi_mcp.go, and shared registry cache invalidation. I have no new findings to add that would not duplicate those.
What is not reviewable
The Go source changes (pkg/workflow/agentic_engine.go, compiler_orchestrator_engine.go, pi_mcp.go, and others) are beyond the 3 000-line patch cap. The existing three inline comments cover the critical issues surfaced there.
Positive Highlights
- ✅ Mechanical lock-file regeneration is consistent across all 280+ workflows
- ✅ Docs update matches the functional removal with no stale references left behind
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21.6 AIC · ⌖ 9.08 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
Review: Refactoring opencode integration for shared workflows
This PR systematically removes the opencode engine from all compiled workflow lock files and updates syntax-engine.md documentation to match.
Scope: 97 lock files + 1 doc file. No Go source changes in this diff.
Assessment: The changes are mechanical and consistent — every lock file removes .opencode from sparse-checkout paths and opencode.jsonc from GH_AW_AGENT_FILES/GH_AW_AGENT_FOLDERS. The documentation is updated accordingly. No issues found in the changed lines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.8 AIC · ⌖ 11.3 AIC · ⊞ 5.4K
Adds a draft Architecture Decision Record documenting the decision to remove the opencode experimental engine from GitHub Agentic Workflows, covering context, alternatives considered, and consequences. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core workflow infrastructure (274 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
/matt review the go code first |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Verdict: Request changes
Core refactor (dropping OpenCode as a built-in engine in favor of behavior-defined engines, deferring engine validation for imported engine definitions) is directionally sound, but the import-declaration detection is incomplete and will misbehave for a documented, actually-used import shape.
💡 Themes
frontmatterDeclaresImportsonly handlesimports: [...]([]any), missing the object formimports: { aw: [...] }that the project's own import parser explicitly supports and that other tests construct as[]string. This defeats the purpose of the new bypass for a real workflow-authoring pattern.- The manifest-cache invalidation on
Registeris correct in intent but introduces unsynchronized writes to global singleton state; currently safe only because registration is sequential — flagging as a latent risk for future parallel compilation work. - The mechanical OpenCode-removal/rename changes across
.lock.ymlfiles and comments/error messages look consistent and low-risk.
🔎 Code quality review by PR Code Quality Reviewer · auto · 125.7 AIC · ⌖ 6.74 AIC · ⊞ 7.9K
Comment /review to run again
| // validation must be deferred until imports have been processed. | ||
| func frontmatterDeclaresImports(frontmatter map[string]any) bool { | ||
| imports, ok := frontmatter["imports"].([]any) | ||
| return ok && len(imports) > 0 |
There was a problem hiding this comment.
This import-detection helper only recognizes the []any array form; the equally-valid object form imports: { aw: [...] } parses to map[string]any and is silently treated as "no imports", so early engine validation runs anyway and can wrongly reject a valid engine defined only in the imported file.
💡 Details and fix
The workflow's own import parser (pkg/parser/import_bfs.go parseImportSpecsFromField) explicitly supports three shapes: []any, []string, and map[string]any (object form with an aw subfield). frontmatterDeclaresImports only checks the first:
func frontmatterDeclaresImports(frontmatter map[string]any) bool {
imports, ok := frontmatter["imports"].([]any)
return ok && len(imports) > 0
}Verified independently: unmarshalling imports:\n aw:\n - a.md with the project's YAML library (goccy/go-yaml) produces map[string]interface{}, not []interface{}. A workflow using imports:\n aw:\n - shared/some-engine.md (or a plain imports: [x.md] decoded elsewhere as []string, which several existing tests construct directly) will bypass the intended deferral and fail the "Early engine validation" check even though the shared file legitimately supplies the engine.
Fix: reuse the same detection logic as the import parser instead of duplicating a partial check, e.g.:
func frontmatterDeclaresImports(frontmatter map[string]any) bool {
switch v := frontmatter["imports"].(type) {
case []any:
return len(v) > 0
case []string:
return len(v) > 0
case map[string]any:
aw, ok := v["aw"]
if !ok {
return false
}
switch aw := aw.(type) {
case []any:
return len(aw) > 0
case []string:
return len(aw) > 0
}
}
return false
}| } | ||
| agenticEngineLog.Printf("Registering engine: id=%s, name=%s", engine.GetID(), engine.GetDisplayName()) | ||
| r.engines[engine.GetID()] = engine | ||
| // Invalidate the pre-computed manifest caches so engines registered after | ||
| // construction (e.g. behavior-defined engines imported from shared workflows) | ||
| // contribute their manifest files and folders. | ||
| r.cachedManifestFolders = nil | ||
| r.cachedManifestFiles = nil |
There was a problem hiding this comment.
Registering an engine now mutates shared registry state (cachedManifestFolders/cachedManifestFiles) with no locking, on a registry documented as a global singleton — a latent data race if compilation is ever parallelized.
|
@copilot update wasm golden |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — large mechanical refactor, overall well-executed with one documentation gap worth noting.
📋 Key Themes & Highlights
What changed
285 lock files uniformly remove .opencode from:
- sparse checkout paths
GH_AW_AGENT_FOLDERSenv varGH_AW_AGENT_FILESenv var (removingopencode.jsonc)
Additionally, syntax-engine.md removes opencode from the documented engine options and its description block.
Positive Highlights
- ✅ Consistent, uniform mechanical change across all affected lock files — no drift between files
- ✅
shared/opencode.mdandsmoke-opencode.lock.ymlremain intact, confirmingopencodeis not being deleted — just centralised into the shared workflow pattern - ✅ The lock files are generated output and the pattern is correct and complete throughout
Concern: Documentation gap in syntax-engine.md
The syntax-engine.md doc now omits opencode as a valid engine identifier, but the engine remains fully functional via imports: [shared/opencode.md]. A user consulting the syntax reference will not know opencode is still available — they just need to import the shared module rather than specifying engine: opencode directly.
Consider adding a note like: "The opencode engine is available as a shared workflow import (shared/opencode.md); see smoke-opencode for an example."
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.4 AIC · ⌖ 8.14 AIC · ⊞ 7.1K
Comment /matt to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed the failing activation tests in e72177b. |
|
/smoke-opencode |
|
🚀 Smoke OpenCode MISSION COMPLETE! OpenCode delivered. 🔥 |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in f9a16cb. The stale OpenCode global-registry assertions were removed; the targeted tests and required validation now pass. |
|
@copilot Please take another pass on this PR. The latest push addressed test failures, but there is still follow-up needed before a maintainer can review efficiently:
Please also confirm the branch is ready for maintainer investigation once those items are done. Run: https://github.com/github/gh-aw/actions/runs/30875655374
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Pull request created by AI Agent